Fix starting/stopping SPN + more #1668
Conversation
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughWalkthroughThe updates encompass enhancements to linting configurations, dependency management, and various function modifications across multiple files. Key changes include an upgrade to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerManager as WM
participant Worker as W
Client->>WM: Start Worker
WM->>W: Initialize Worker
W->>WM: Worker Ready
WM->>Client: Worker Started
Client->>WM: Get Worker Info
WM->>W: Retrieve Status
W-->>WM: Status Data
WM-->>Client: Return Worker Info
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range comments (1)
service/profile/config-update.go (1)
Line range hint
36-36: Implement error handling as indicated by TODO comments.Several parts of the
updateGlobalConfigProfilefunction have TODO comments related to error handling. It is crucial to implement these to prevent potential runtime errors and ensure the robustness of the configuration update process.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (21)
- .github/workflows/go.yml (1 hunks)
- .golangci.yml (2 hunks)
- base/rng/tickfeeder.go (2 hunks)
- go.mod (1 hunks)
- service/core/api.go (1 hunks)
- service/core/core.go (2 hunks)
- service/debug.go (1 hunks)
- service/debug_test.go (1 hunks)
- service/instance.go (1 hunks)
- service/mgr/events.go (2 hunks)
- service/mgr/group.go (1 hunks)
- service/mgr/manager.go (4 hunks)
- service/mgr/states.go (1 hunks)
- service/mgr/worker.go (3 hunks)
- service/mgr/worker_info.go (1 hunks)
- service/mgr/worker_test.go (1 hunks)
- service/mgr/workermgr.go (3 hunks)
- service/profile/config-update.go (1 hunks)
- service/profile/module.go (2 hunks)
- spn/access/module.go (1 hunks)
- spn/debug.go (1 hunks)
Additional comments not posted (31)
service/debug_test.go (1)
16-25: Error handling is robust.Proper error handling with
t.Fatalensures that the test stops executing when an error occurs, which is good practice.service/mgr/worker_test.go (1)
29-51: Good isolation of test functions.Each worker test function is well isolated, ensuring that they do not interfere with each other's execution. This is good practice in writing maintainable and reliable tests.
.github/workflows/go.yml (1)
43-43: Update togolangci-lintversion is appropriate.Updating the
golangci-lintaction tov1.60.3is a good practice to keep the tools up-to-date with the latest improvements and bug fixes. Ensure that this version is compatible with the project's existing linter configuration and Go version.Run the following script to verify the compatibility of the new linter version with the project's setup:
spn/debug.go (1)
32-49: Review: Implementation ofAddWorkerInfoToDebugInfomethod.This method adds worker information to the debug info. The error handling and conditional logic from lines 35-41 are well implemented, ensuring that any errors in fetching worker info are captured and reported correctly. The formatting of the worker status in line 45 is clear and informative.
The implementation here is robust and follows good practices in error handling and user feedback.
base/rng/tickfeeder.go (1)
41-49: Review: RefactoredtickFeederfunction usingtime.Ticker.The refactoring of the
tickFeederfunction to usetime.Tickerfor managing tick intervals is a significant improvement. This change allows the function to be more responsive to cancellation signals, as it no longer relies on the potentially blockingtime.Sleep.The implementation is efficient and aligns with best practices for handling time-based operations in Go. The use of
selectfor listening to ticker and context cancellation is correctly implemented.service/debug.go (1)
36-53: Review: Implementation ofAddWorkerInfoToDebugInfomethod inservicepackage.This method is well implemented, similar to its counterpart in the
spnpackage. The error handling and user feedback mechanisms are robust and follow good practices.The method is correctly implemented and follows best practices.
.golangci.yml (1)
21-21: Approved: Addition of new linters.Adding
gomoddirectives,mnd, andtestifylintenhances the project's focus on clean, maintainable code and effective testing practices.Also applies to: 27-27, 39-39
service/core/core.go (2)
11-11: Approved: Addition of debug package.The new import
github.com/safing/portmaster/base/utils/debugaligns with the PR's goal to enhance debugging capabilities.
116-116: Approved: Addition of new method toinstanceinterface.Adding
AddWorkerInfoToDebugInfo(di *debug.Info)to theinstanceinterface enhances the system's debugging and observability by allowing worker-specific data to be included in debug information.service/profile/module.go (1)
117-121: Approved: Addition of configuration profile updater registration.The new code block in the
startfunction to register a global configuration profile updater enhances the module's robustness by ensuring timely response to configuration events.service/profile/config-update.go (2)
26-26: Function name change enhances clarity.The renaming of
registerConfigUpdatertoregisterGlobalConfigProfileUpdaterimproves the clarity and descriptiveness of the function's purpose.
Line range hint
36-36: Proper use of locking mechanism.The use of
cfgLockto manage concurrent access to the configuration settings is appropriate and ensures thread safety.service/mgr/states.go (2)
153-156: Enhanced thread safety in theClearmethod.The changes to defer the unlocking of
statesLockuntil after the event submission in theClearmethod enhance thread safety and prevent potential race conditions.
155-156: Updated comment improves clarity.The updated comment in the
Clearmethod clarifies why the event submission occurs without holding the lock, which is beneficial for maintainability and understanding the rationale behind the concurrency control.service/mgr/events.go (2)
109-153: Robustness and efficiency improvements in theSubmitmethod.The addition of checks for canceled callbacks and the use of asynchronous execution for callbacks in the
Submitmethod enhance the robustness and efficiency of the event handling mechanism.
126-153: Refined error handling in callback execution.The refined error handling and management of the
cancelflag in theSubmitmethod improve the clarity and reliability of error reporting, enhancing the overall robustness of the event management system.go.mod (1)
93-93: Approved addition of new dependency.The addition of
github.com/maruel/panicparse/v2as an indirect dependency is a sensible choice for enhancing debugging capabilities by handling panic stack traces more effectively.service/mgr/manager.go (2)
26-28: Enhanced worker management in Manager struct.The addition of
workers,workersIndex, andworkersLockfields, along with their initialization innewManager, significantly enhances the management of worker contexts, ensuring better thread safety and structured handling.Also applies to: 41-41
205-206: Refined worker lifecycle management methods.The changes to
workerStartandworkerDonemethods to accept a*WorkerCtxparameter improve clarity and enforce that operations are tied to specific worker instances, enhancing the robustness of worker management.Also applies to: 210-211
spn/access/module.go (1)
89-92: Optimized control flow and enhanced logging in SPN management.Moving the
loadTokens()function to occur after the SPN enablement check avoids unnecessary operations. The added logging provides clear feedback on the SPN's operational state, enhancing observability.service/mgr/workermgr.go (1)
234-248: Review of the newStatusmethod.The
Statusmethod implementation uses locking to access shared resources, which is essential for thread safety. The method correctly locks and unlocks around the shared state access, which prevents race conditions.The method returns a string representation of the current state, which is useful for monitoring and debugging. The use of a switch statement to determine the state is clear and efficient.
Overall, the implementation looks solid and should function as expected without any performance concerns given the typical usage patterns of such a method.
The code changes are approved.
service/core/api.go (1)
154-154: Enhancement ofdebugInfowith worker information.The addition of
module.instance.AddWorkerInfoToDebugInfo(di)to thedebugInfofunction is a valuable enhancement, providing more detailed insights into the worker's state during debugging. This change allows for better troubleshooting and monitoring of the system's internal workings.Ensure that the method
AddWorkerInfoToDebugInfohandles data sensitively, especially if it includes information that could be considered private or sensitive. It's also important to ensure that this method does not significantly impact the performance of the debugging process.The code changes are approved, assuming the method handles data appropriately and efficiently.
service/mgr/worker.go (3)
24-25: Enhancement to WorkerCtx struct approved.The addition of
nameandworkFuncfields enhances the functionality by allowing the worker context to carry more specific information about its operation.The changes to the
WorkerCtxstruct are approved.
168-176: Proper initialization and management in manageWorker method.The method correctly initializes the
WorkerCtxwith the new fields and uses them to manage the worker's lifecycle effectively.The changes to the
manageWorkermethod are approved.
254-262: Consistent worker context management in Do method.The method mirrors the changes in
manageWorker, ensuring consistency across worker management functions.The changes to the
Domethod are approved.service/mgr/worker_info.go (5)
23-52: Well-implemented worker registration logic.The function handles the addition of workers to a ring buffer efficiently, including handling ring buffer expansion when needed.
The changes to the
registerWorkerfunction are approved.
54-74: Efficient worker unregistration logic.The function efficiently handles the removal of workers from the ring buffer, iterating backwards to find the worker to remove.
The changes to the
unregisterWorkerfunction are approved.
98-201: Comprehensive worker status retrieval logic.The function provides detailed insights into the status of workers using a stack snapshot, which is a robust method for gathering detailed information.
The changes to the
WorkerInfofunction are approved.
203-234: User-friendly formatting of worker information.The function uses a tab writer to format the worker information into a readable table, enhancing user-friendliness.
The changes to the
Formatfunction are approved.
252-275: Efficient merging and deduplication of worker information.The function efficiently merges multiple worker info objects into one, handling the aggregation and deduplication of worker details.
The changes to the
MergeWorkerInfofunction are approved.service/instance.go (1)
622-622: Corrected logic in Stopping method.The change to return
truewhen the context's error is notnilcorrectly reflects the shutdown state, enhancing the clarity and correctness of the method.The changes to the
Stoppingmethod are approved.
| func TestDebug(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Create test instance with at least one worker. | ||
| i := &Instance{} | ||
| n, err := notifications.New(i) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| i.serviceGroup = mgr.NewGroup(n) | ||
| i.SpnGroup = mgr.NewExtendedGroup() | ||
| err = i.Start() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
|
|
||
| info, err := i.GetWorkerInfo() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Log(info) | ||
| } |
There was a problem hiding this comment.
Consider using synchronization primitives instead of time.Sleep in tests.
The use of time.Sleep in tests (line 26) can lead to non-deterministic behavior and make the tests flaky. Consider using synchronization primitives like channels or wait groups to synchronize the operations more reliably.
| // Create test instance with at least one worker. | ||
| i := &Instance{} | ||
| n, err := notifications.New(i) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| i.serviceGroup = mgr.NewGroup(n) | ||
| i.SpnGroup = mgr.NewExtendedGroup() | ||
| err = i.Start() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Ensure proper cleanup in tests.
The test creates an instance and starts it but does not ensure it is properly stopped or cleaned up after the test, which could lead to resource leaks during test runs.
Consider adding a defer statement to stop the instance:
+ defer i.Stop()
err = i.Start()
if err != nil {
t.Fatal(err)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create test instance with at least one worker. | |
| i := &Instance{} | |
| n, err := notifications.New(i) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| i.serviceGroup = mgr.NewGroup(n) | |
| i.SpnGroup = mgr.NewExtendedGroup() | |
| err = i.Start() | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| // Create test instance with at least one worker. | |
| i := &Instance{} | |
| n, err := notifications.New(i) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| i.serviceGroup = mgr.NewGroup(n) | |
| i.SpnGroup = mgr.NewExtendedGroup() | |
| defer i.Stop() | |
| err = i.Start() | |
| if err != nil { | |
| t.Fatal(err) | |
| } |
| func TestWorkerInfo(t *testing.T) { //nolint:paralleltest | ||
| mgr := New("test") | ||
| mgr.Go("test func one", testFunc1) | ||
| mgr.Go("test func two", testFunc2) | ||
| mgr.Go("test func three", testFunc3) | ||
| defer mgr.Cancel() | ||
|
|
||
| time.Sleep(100 * time.Millisecond) | ||
|
|
||
| info, err := mgr.WorkerInfo(nil) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if info.Waiting != 3 { | ||
| t.Errorf("expected three waiting workers") | ||
| } | ||
|
|
||
| fmt.Printf("%+v\n", info) | ||
| } |
There was a problem hiding this comment.
Consider handling potential race conditions or using more deterministic synchronization.
The test does not run in parallel, which is likely due to potential race conditions with shared resources. However, using time.Sleep (line 16) to wait for worker readiness is not ideal as it can lead to flaky tests. Consider using more deterministic synchronization mechanisms.
| info, err := mgr.WorkerInfo(nil) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if info.Waiting != 3 { | ||
| t.Errorf("expected three waiting workers") | ||
| } |
There was a problem hiding this comment.
Add more comprehensive assertions for robust testing.
The test checks if the number of waiting workers is exactly three, but it does not validate other important aspects of the worker info, such as active workers or completed tasks. Consider adding more assertions to cover these aspects.
| // GetWorkerInfo returns the worker info of all running workers. | ||
| func (i *Instance) GetWorkerInfo() (*mgr.WorkerInfo, error) { | ||
| snapshot, _, err := stack.ScanSnapshot(bytes.NewReader(fullStack()), io.Discard, stack.DefaultOpts()) | ||
| if err != nil && !errors.Is(err, io.EOF) { | ||
| return nil, fmt.Errorf("get stack: %w", err) | ||
| } | ||
|
|
||
| infos := make([]*mgr.WorkerInfo, 0, 32) | ||
| for _, m := range i.serviceGroup.Modules() { | ||
| wi, _ := m.Manager().WorkerInfo(snapshot) // Does not fail when we provide a snapshot. | ||
| infos = append(infos, wi) | ||
| } | ||
|
|
||
| return mgr.MergeWorkerInfo(infos...), nil | ||
| } |
There was a problem hiding this comment.
Review: Implementation of GetWorkerInfo method.
This method retrieves worker information from all running workers. The use of stack.ScanSnapshot for capturing the stack snapshot is appropriate here. However, there are a few points to consider:
- The error handling in line 19 could be enhanced by logging the error or providing more detailed feedback.
- The loop in lines 24-27 iterates over modules and fetches worker info without handling potential errors explicitly. It's crucial to handle or log these errors to avoid silent failures.
Consider enhancing error handling and logging within this method to ensure robustness and easier debugging. Here's a suggested improvement:
- wi, _ := m.Manager().WorkerInfo(snapshot)
+ wi, err := m.Manager().WorkerInfo(snapshot)
+ if err != nil {
+ return nil, fmt.Errorf("error fetching worker info: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetWorkerInfo returns the worker info of all running workers. | |
| func (i *Instance) GetWorkerInfo() (*mgr.WorkerInfo, error) { | |
| snapshot, _, err := stack.ScanSnapshot(bytes.NewReader(fullStack()), io.Discard, stack.DefaultOpts()) | |
| if err != nil && !errors.Is(err, io.EOF) { | |
| return nil, fmt.Errorf("get stack: %w", err) | |
| } | |
| infos := make([]*mgr.WorkerInfo, 0, 32) | |
| for _, m := range i.serviceGroup.Modules() { | |
| wi, _ := m.Manager().WorkerInfo(snapshot) // Does not fail when we provide a snapshot. | |
| infos = append(infos, wi) | |
| } | |
| return mgr.MergeWorkerInfo(infos...), nil | |
| } | |
| // GetWorkerInfo returns the worker info of all running workers. | |
| func (i *Instance) GetWorkerInfo() (*mgr.WorkerInfo, error) { | |
| snapshot, _, err := stack.ScanSnapshot(bytes.NewReader(fullStack()), io.Discard, stack.DefaultOpts()) | |
| if err != nil && !errors.Is(err, io.EOF) { | |
| return nil, fmt.Errorf("get stack: %w", err) | |
| } | |
| infos := make([]*mgr.WorkerInfo, 0, 32) | |
| for _, m := range i.serviceGroup.Modules() { | |
| wi, err := m.Manager().WorkerInfo(snapshot) | |
| if err != nil { | |
| return nil, fmt.Errorf("error fetching worker info: %w", err) | |
| } | |
| infos = append(infos, wi) | |
| } | |
| return mgr.MergeWorkerInfo(infos...), nil | |
| } |
| func fullStack() []byte { | ||
| buf := make([]byte, 8096) | ||
| for { | ||
| n := runtime.Stack(buf, true) | ||
| if n < len(buf) { | ||
| return buf[:n] | ||
| } | ||
| buf = make([]byte, 2*len(buf)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Review: fullStack function implementation in service package.
The implementation is identical to that in the spn package. The dynamic buffer resizing strategy is appropriate, but a comment for clarity would be beneficial.
Add a comment explaining the buffer resizing strategy as suggested for the spn/debug.go file.
| @@ -18,11 +18,13 @@ linters: | |||
| - gocyclo | |||
| - goerr113 | |||
| - gomnd | |||
| - gomoddirectives | |||
There was a problem hiding this comment.
Issue: Contradictory linter configuration.
The linters perfsprint, testifylint, and gomoddirectives are both added and removed. This could be an oversight. Please verify the linter configuration.
Also applies to: 27-27, 39-39
| @@ -23,7 +23,7 @@ var ( | |||
| cfgFilterLists []string | |||
| ) | |||
|
|
|||
| func registerConfigUpdater() error { | |||
| func registerGlobalConfigProfileUpdater() error { | |||
There was a problem hiding this comment.
Consider removing the unused struct parameter.
The callback function within registerGlobalConfigProfileUpdater takes a struct parameter that is not used. Consider removing it if it serves no purpose, to simplify the function signature.
| wCtx.workerMgr = s | ||
|
|
||
| go s.taskMgr() | ||
| return s | ||
| } | ||
|
|
||
| func (s *WorkerMgr) taskMgr() { | ||
| s.mgr.workerStart() | ||
| defer s.mgr.workerDone() | ||
| s.mgr.workerStart(s.ctx) | ||
| defer s.mgr.workerDone(s.ctx) |
There was a problem hiding this comment.
Refactor suggestion for context handling in taskMgr.
The addition of the ctx parameter in taskMgr is a good practice for better control over goroutines, especially for cancellation. However, the method starts with s.mgr.workerStart(s.ctx) which uses the manager's context (s.ctx) instead of the passed context (ctx). This might lead to unexpected behavior if the passed context is different from the manager's context.
Consider using the passed ctx for all operations within taskMgr to ensure consistent behavior.
- func (s *WorkerMgr) taskMgr() {
+ func (s *WorkerMgr) taskMgr(ctx context.Context) {
- s.mgr.workerStart(s.ctx)
+ s.mgr.workerStart(ctx)
- defer s.mgr.workerDone(s.ctx)
+ defer s.mgr.workerDone(ctx)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wCtx.workerMgr = s | |
| go s.taskMgr() | |
| return s | |
| } | |
| func (s *WorkerMgr) taskMgr() { | |
| s.mgr.workerStart() | |
| defer s.mgr.workerDone() | |
| s.mgr.workerStart(s.ctx) | |
| defer s.mgr.workerDone(s.ctx) | |
| wCtx.workerMgr = s | |
| go s.taskMgr(wCtx) | |
| return s | |
| } | |
| func (s *WorkerMgr) taskMgr(ctx context.Context) { | |
| s.mgr.workerStart(ctx) | |
| defer s.mgr.workerDone(ctx) |
| @@ -30,6 +30,7 @@ const ( | |||
| groupStateInvalid | |||
| ) | |||
|
|
|||
| //nolint:goconst | |||
There was a problem hiding this comment.
Consider the implications of using //nolint:goconst.
The addition of the //nolint:goconst directive suppresses lint warnings related to the use of constant values in the groupStateToString function. While this can be justified if the constants are essential for the function's logic, it's important to ensure that this does not encourage the use of magic numbers or other hard-coded values that could reduce code maintainability.
If the constants are well-defined and used appropriately within the function, this directive is acceptable. However, consider documenting the reason for this suppression to maintain clarity for future maintenance.
Consider adding a comment explaining why //nolint:goconst is necessary here to aid future developers.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores